Skip to content

Conversation

@elianiva
Copy link
Contributor

@elianiva elianiva commented Aug 17, 2025

Related GitHub Issue

Closes: #3788
Closes: #7173

Roo Code Task Context (Optional)

Description

Currently we're checking if directory is writable by writing a file there and removing it, this could provide false-negative result. This PR changes it to use fs.access instead so it is more consistent between runs.

Test Procedure

Tried to run some sessions with the new changes and it seems to be fixed. I also added a new test, though it currently only checks on mocked fs if fs.access call fails and whether or not it should default path / show warning instead of checking with the actual filesystem, not exactly sure what to do here.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

Documentation Updates

Additional Notes

Get in Touch

@elianiva


Important

Refactor directory writability check in storage.ts using fs.access and add tests in storage.spec.ts.

  • Behavior:
    • Refactor getStorageBasePath in storage.ts to use fs.access for checking directory writability instead of creating and deleting a test file.
    • Update promptForCustomStoragePath in storage.ts to use fs.access for path validation.
  • Tests:
    • Add tests in storage.spec.ts to verify behavior when custom path is writable and when it is not.
    • Mock fs/promises to simulate different filesystem scenarios.

This description was created by Ellipsis for ba92646. You can customize this summary. It will automatically update as commits are pushed.

@elianiva elianiva requested review from cte, jr and mrubens as code owners August 17, 2025 22:44
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Aug 17, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! I've reviewed the changes and have some suggestions for improvement. Overall, this is a good improvement that addresses issue #3788 - the change from creating/deleting a test file to using fs.access is more reliable and cleaner.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 17, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 17, 2025
@jimweller
Copy link

I like this approach better than PR # 7175 and PR # 7174

#3788
#7173

Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @elianiva! This looks good!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 18, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Aug 19, 2025
@hannesrudolph hannesrudolph added PR - Needs Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Aug 19, 2025
@mrubens mrubens merged commit ce052db into RooCodeInc:main Aug 21, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 21, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer PR - Needs Review size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

getStorageBasePath(), race condition customStoragePath setting does not work - files always saved in default location

5 participants